fix(coordinator): prioritize event label intent and sync push provider lock state - #701

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.99%. Comparing base (cdb4922) to head (6963343).
⚠️ Report is 229 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #701      +/-   ##
==========================================
+ Coverage   84.14%   93.99%   +9.85%     
==========================================
  Files          10       42      +32     
  Lines         801     5433    +4632     
  Branches        0       30      +30     
==========================================
+ Hits          674     5107    +4433     
- Misses        127      326     +199     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Thanks for highlighting the 3 blockers in the review summary:

1. Precedence order in _handle_provider_lock_event: Updated _handle_provider_lock_event in commit 1f9db53 so explicit "unlock" / "lock" intent in event_label is evaluated before checking state_changed. Updated test_handle_provider_lock_event_prioritizes_event_label to set a stale kmlock.lock_state (UNLOCKED while entity state is LOCKED) to verify event label precedence when state_changed is true.

2. Push provider scope: Added test coverage in commit 23abe6d for non-push providers (supports_push_updates = False) and provider = None via test_handle_lock_state_change_non_push_provider_does_not_sync_state to ensure kmlock.lock_state is only mutated for push-capable providers.

3. Refactor: Pipeline Keymaster refreshes through dirty lock scopes #695 Interaction: Verified compatibility with notifications scheduling across provider updates.

All 167 coordinator tests pass cleanly.

main has moved; this now conflicts only in custom_components/keymaster/coordinator.py inside _handle_lock_state_change, while tests/test_coordinator.py auto-merges cleanly. #695 removed async_schedule_global_notification() on main; resolve both hunks by keeping main's scoped self.async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id]) and also keeping the uses_provider_lock_events state-sync additions, because taking ours-only drops the push-provider fix and taking theirs-only calls a removed method/reintroduces global fan-out.

<<<<<<< upstream/main
                    self.async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id])
=======
                    self.async_schedule_global_notification()
                if uses_provider_lock_events:
                    kmlock.lock_state = LockState.UNLOCKED
                    self._last_unlock_code_slot[kmlock.keymaster_config_entry_id] = 0
>>>>>>> upstream/pr/701
<<<<<<< upstream/main
                    self.async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id])
=======
                    self.async_schedule_global_notification()
                if uses_provider_lock_events:
                    kmlock.lock_state = LockState.LOCKED
>>>>>>> upstream/pr/701

Thanks @tykeal! Applied the cleanup in 808b27e:

• Removed test_handle_provider_lock_event_prioritizes_event_label from test_coordinator.py to eliminate the redundant test targeting unchanged code in _handle_provider_lock_event.
• Retained the regression tests for push provider lock state sync in _handle_lock_state_change.

Re-verified at 808b27ec7e409579aaf0543180caa23e86338626.

Blocker 4 is resolved — test_handle_provider_lock_event_prioritizes_event_label is removed, and it cost nothing: _handle_provider_lock_event retains full coverage (L904-969, 19 statements, 0 missing) and coordinator.py reports an identical 1404/139/90% at both heads. Patch coverage of the five changed lines is still 100%.

Blockers 1, 2 and 3 are unaddressed. coordinator.py is byte-identical between 09f770d5 and 808b27ec:

$ git cat-file blob 09f770d5:custom_components/keymaster/coordinator.py | sha256sum
cb32e469a198da54970ad823fd9e44cc5f1d6631bd595874bc2e4b70fc9198ad
$ git cat-file blob 808b27ec:custom_components/keymaster/coordinator.py | sha256sum
cb32e469a198da54970ad823fd9e44cc5f1d6631bd595874bc2e4b70fc9198ad

so the five added lines in _handle_lock_state_change are unchanged from what was reviewed. I re-ran the three probes at this head against merge base 83a572e; all three still reproduce with the same output as recorded in #701 (review):

• L1039-1040 — provider lock event loses the keymaster_lock_state_changed bus event, the lock notification and all three dismiss_persistent_notification() calls when the state change lands first.
• L1021-1023 — code_slot_num == 0 provider unlock events (Z-Wave JS manual/RF, providers/zwave_js.py:976, :1057) produce no bus event and no notification.
• L1023 — the unconditional slot-tracker reset clobbers an already-recorded slot in the provider-first ordering, yielding duplicate events and notifications.

Details and before/after output are in that review; I won't repeat them here.

Local tooling at this head is clean: 1059 passed, ruff check clean, ruff format --check clean, mypy clean.

The four red checks are infrastructure, not defects. coverage, Autolabel PR, HACS Validation and Hassfest Validation all have conclusion: cancelled with runner_name: "", steps: [] and no log blob (/actions/jobs/<id>/logs returns BlobNotFound) — they never got a runner and hit the 15-minute queue timeout, which is why all four show an identical 15m01s. #702's Python CI and Validation And Formatting are still queued from the same window. Re-run when runners free up; there is no coverage regression behind the coverage check.

Also still open, minor: the supports_push parameter in the parametrize on test_handle_lock_state_change_non_push_provider_does_not_sync_state is unused and False in both cases.

Thanks for the thorough review and edge-case analysis! @tykeal

I've updated the implementation in commit 180f29a to ensure push provider lock and unlock events execute their full side effects regardless of event arrival order relative to Home Assistant lock entity state changes:

1. Pending Provider Event Tracking:

   • Added _pending_provider_unlock_event and _pending_provider_lock_event sets to KeymasterCoordinator to track state transitions initiated by entity state changes awaiting provider callbacks.
   • In _handle_lock_state_change, state changes register pending provider events and use _last_unlock_code_slot.setdefault(...) so existing slot attributions aren't clobbered if provider events arrive first.

2. Unlock Path Order Independence:

   • In _lock_unlocked, pending provider events bypass early returns. Slot 0 (code_slot_num == 0) unlock events, slot > 0 unlock events, access limit count decrements (accesslimit_count), slot updates, global/slot notifications, and keymaster_lock_state_changed bus events now execute fully even if entity state changes land first.

3. Lock Path Order Independence:

   • In _lock_locked, pending provider events bypass early returns. Lock events following state changes now reliably fire the keymaster_lock_state_changed bus event (ATTR_STATE: LOCKED), send notifications, and dismiss persistent autolock notifications (dismiss_persistent_notification).

4. Test Coverage & Cleanup:

   • Cleaned up unreferenced parametrization in test_handle_lock_state_change_non_push_provider_does_not_sync_state.
   • Added unit tests covering slot 0 unlock events, slot > 0 access limit decrements, lock event notifications/dismissals, and provider-first event ordering.

Addressed the non-blocking follow-up items in commit 855d1a4:

• Throttle Alignment: Moved _pending_provider_unlock_event.discard(...) below the throttle check in _lock_unlocked so its lifecycle behavior aligns with _lock_locked.
• Delete Lock Cleanup: Added discard calls for _pending_provider_unlock_event and _pending_provider_lock_event in _delete_lock.
• Unit Tests: Added tests for duplicate provider lock event suppression and _delete_lock pending event set cleanup.

Re-verified at 855d1a41.

Follow-ups 2 and 3 are resolved. The _delete_lock discards are correctly placed alongside the sibling cleanups and are covered by test_delete_lock_clears_pending_provider_event_sets. async_shutdown and config-entry unload do not need them — neither clears _last_unlock_code_slot or _state_change_autolock_started either, since the coordinator object is discarded wholesale, so _delete_lock is the consistent and sufficient teardown path.

The discard move below the throttle gate is correct and does not open the failure mode it might appear to. A throttled provider event now returns with the token still armed, but that is the right semantics: the token means "side effects not yet emitted for this transition", and a throttled event emits none, so the next event correctly consumes it on the full-effect path. Both paths are now consistent and both match base.

Probes A/B/C still match base (bus=1 notify=1 dismiss=3; bus=1 notify=1 slot=0; tracker 3->3 with one bus event and one notification). A 26-sequence differential matrix of push-provider event orderings against 83a572e gives 22 identical and 4 divergent, all four being this PR emitting the lock-side bus event, notification and three dismissals that base drops.

Correcting a number I published in pullrequestreview-4878019629: the matrix result is 22/4, not the 21/5 I stated there. Bus-event ordering is nondeterministic across runs in my probe harness — three runs at a fixed commit produced different orderings, including for a sequence that touches no token code at all, which establishes it as a harness artifact rather than a behavioural difference. Compared order-insensitively the result is a stable 22 identical / 4 divergent, and the four divergences are the same substantive ones as before.

Follow-up 1 is half-covered. test_duplicate_provider_lock_event_suppressed_after_state_change catches removal of the lock-side discard, and it is a genuine test — it asserts notification and bus-event counts rather than internal set membership. The unlock direction has no equivalent. Deleting self._pending_provider_unlock_event.discard(...) in _lock_unlocked leaves the full suite green, while a state change to UNLOCKED followed by two provider unlock events then produces two bus events and two notifications instead of one. A mirror test would close that. Non-blocking; it does not gate merge.

The mutual cross-discards in _handle_lock_state_change are equivalent mutants under all eight oscillation sequences I probed — removing either produces no observable difference in bus, notification, dismiss or slot-tracker state. A stale opposite-direction token cannot fire its guard, because that guard requires the lock state to already match and the handler has just forced it the other way. The absence of a test there is correct rather than a gap.

pytest tests/: 1065 passed, 1 deselected. ruff check, ruff format --check and mypy custom_components/keymaster/ are clean. 100% coverage of the changed lines, none uncovered. Merges cleanly into main.

No CI has run on this head. commits/855d1a41/status returns {"state":"pending","total":0} with zero check-runs, and no workflow run object was created for either 180f29ac or 855d1a41 — the most recent runs on the branch are still against 808b27ec. All ten workflows are state: active, and a genuine skip still materialises a run with conclusion: skipped, so this is dispatch failure from the same Actions incident as the earlier queue timeouts. Note that mergeStateStatus: CLEAN currently reflects absent checks rather than passing ones; closing and reopening the PR would re-trigger the pull_request workflows and produce a real signal.

Added test_duplicate_provider_unlock_event_suppressed_after_state_change in commit e9c6746 to complete test coverage for the unlock-side token discard behavior as well. @tykeal

Re-verified at e9c6746a. Test-only: coordinator.py is byte-identical to 855d1a41 (sha256 860f92a5...), so the differential matrix and ordering probes carry over unchanged.

M6 is now caught. Deleting self._pending_provider_unlock_event.discard(...) from _lock_unlocked fails test_duplicate_provider_unlock_event_suppressed_after_state_change. Full battery re-run: M1, M2, M4, M6, M7, M9 all caught; M3 and the guard flip survive individually but are jointly load-bearing with each other; M8a/M8b remain equivalent mutants as previously established. The new test masks nothing.

One minor note. Both duplicate-suppression tests kill their mutants through the internal set-membership assertion rather than the bus-event and notification counts. Removing just that assertion leaves each test passing under its mutation, because both events in each test are the same direction, so nothing resets the throttle between them and THROTTLE_SECONDS suppresses the duplicate regardless of the token. With the throttle mocked to always allow, M6 shows bus=2 notify=2 against bus=1 notify=1 unmutated. Patching _throttle.is_allowed in both tests would make the count assertions load-bearing. The mutants die either way, so this is not blocking.

pytest: 1066 passed. ruff check and ruff format --check clean. Merges cleanly into main.

Thanks @tykeal! Applied the suggested improvement in commit 6963343:

• Patched coordinator._throttle.is_allowed (return_value=True) in step 3 of test_duplicate_provider_lock_event_suppressed_after_state_change and test_duplicate_provider_unlock_event_suppressed_after_state_change.
• This explicitly bypasses throttle suppression so the event and notification count assertions (mock_notify.call_count == 0 and len(bus_events) == 1) directly and independently verify duplicate suppression by token consumption.

Re-verified at 69633439. The rebase over the #702 merge is clean — merge base is main, #702's files are byte-identical to it, and coordinator.py is unchanged.

The count assertions are now load-bearing: with the internal set assertions removed, M6 and M7 are still caught by the observable notification and bus-event counts. pytest 1077 passed, ruff check and ruff format --check clean, merges cleanly into main.

No open items from my side.